Add a `ChainTipChange` type to `await` chain tip changes #2715

teor2345 · 2021-08-31T08:01:53Z

Motivation

Some tasks need to wait for the state's best chain tip to change.

Solution

crate a basic ChainTipChange implementation
- expand existing tests to cover the new type
rename some types and variables to avoid confusion

This PR is part of ticket #2639, but it doesn't close that ticket.

Review

@jvff can review this PR.

Reviewer Checklist

Code implements Specs and Designs
Tests for Expected Behaviour
Tests for Errors

Follow Up Work

Implement TipAction::Reset in ChainTipChange - #2639

`fastmod ChainTipReceiver CurrentChainTip zebra*`

Also includes the following name changes: ``` fastmod CurrentChainTip LatestChainTip zebra* fastmod chain_tip_receiver latest_chain_tip zebra* ```

jvff

Overall it looks good. The only design related thing that could be discussed is if it would make sense to have only one type. I.e., moving the methods from LatestChainTip into ChainTipChange. It has the advantage of being less code and possibly requiring less mental context, but it does have the disadvantage of having two purposes for the same type, which is okay but could lead to increasing the mental context depending on what scenarios it is used. I'm fine either way, I just wanted to bring it up in case it helps.

jvff · 2021-08-31T10:27:29Z

zebra-state/src/arbitrary.rs

+        let PreparedBlock {
+            block: _,
+            hash,
+            height,
+            new_outputs: _,
+            transaction_hashes,
+        } = prepared;
+        Self {
+            hash,
+            height,
+            transaction_hashes,
+        }


Question: I'm wondering if separating the binding from the construction is a matter of style or if there's some advantage I missed. Like, is this used to be able to capture when new fields are added to PreparedBlock in the future? Just for comparison, my default approach would be something like:

Suggested change

let PreparedBlock {

block: _,

hash,

height,

new_outputs: _,

transaction_hashes,

} = prepared;

Self {

hash,

height,

transaction_hashes,

}

Self {

hash: prepared.hash,

height: prepared.height,

transaction_hashes: prepared.transaction_hashes,

}

I think it's a matter of style, so there's no need to change it. I'm just curious if there's a reason or if there are certain situations where it's preferable to write the destructuring separately 🤔

It's used to avoid cloning the Arcs, which is slightly more expensive due to the atomic reference count.

I've also found it useful to explicitly show all the fields, and show which fields we're dropping.

zebra-state/src/service/chain_tip.rs

jvff · 2021-08-31T11:39:53Z

One more design thing. The ChainTipChange type could impl Stream, which would allow using the StreamExt methods. But I'm not sure if we'd ever need it 😅

Edit: This might need to wrap the watch::Receiver in a tokio_stream::wrappers::WatchStream, so I'm not sure if it's worth it 🤔

teor2345 · 2021-08-31T23:34:32Z

One more design thing. The ChainTipChange type could impl Stream, which would allow using the StreamExt methods. But I'm not sure if we'd ever need it 😅

Edit: This might need to wrap the watch::Receiver in a tokio_stream::wrappers::WatchStream, so I'm not sure if it's worth it 🤔

I deliberately named next after the existing stream and iterator methods.

~~We can turn it into a stream, if we decide that would be useful in a later PR.~~

Edit: actually, I don't think that would be helpful, because it's conceptually different.

teor2345 · 2021-08-31T23:37:05Z

The only design related thing that could be discussed is if it would make sense to have only one type. I.e., moving the methods from LatestChainTip into ChainTipChange. It has the advantage of being less code and possibly requiring less mental context, but it does have the disadvantage of having two purposes for the same type, which is okay but could lead to increasing the mental context depending on what scenarios it is used.

They're actually quite different types, I'll see if I can update the docs and names to make that clearer.

teor2345 · 2021-09-01T00:11:13Z

They're actually quite different types, I'll see if I can update the docs and names to make that clearer.

@jvff what do you think of 09f451e?
Does it help clarify the differences?

teor2345 · 2021-09-01T09:30:47Z

The only design related thing that could be discussed is if it would make sense to have only one type. I.e., moving the methods from LatestChainTip into ChainTipChange. It has the advantage of being less code and possibly requiring less mental context, but it does have the disadvantage of having two purposes for the same type, which is okay but could lead to increasing the mental context depending on what scenarios it is used.

Hopefully the code at https://github.com/ZcashFoundation/zebra/pull/2721/files#r700043308 also clarifies this question.

jvff · 2021-09-01T10:21:49Z

@jvff what do you think of 09f451e?
Does it help clarify the differences?

Yep, thanks!

teor2345 added 3 commits August 31, 2021 18:00

Rename ChainTipReceiver to CurrentChainTip

1963d74

`fastmod ChainTipReceiver CurrentChainTip zebra*`

Update chain tip documentation and variable names

ddd8fc8

Basic chain tip change implementation, without resets

cd490cb

Also includes the following name changes: ``` fastmod CurrentChainTip LatestChainTip zebra* fastmod chain_tip_receiver latest_chain_tip zebra* ```

teor2345 added A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement P-Medium labels Aug 31, 2021

teor2345 requested a review from jvff August 31, 2021 08:01

teor2345 self-assigned this Aug 31, 2021

teor2345 force-pushed the chain-change-grow branch from 5836db9 to cd490cb Compare August 31, 2021 08:01

teor2345 changed the title ~~Add a ChainTipChange type, to await state best chain tip changes~~ Add a ChainTipChange type to await chain tip changes Aug 31, 2021

teor2345 mentioned this pull request Aug 31, 2021

Activate the chain update task when the mempool is activated #2623

Closed

Appease clippy in the tests

4a0c75d

teor2345 mentioned this pull request Aug 31, 2021

Get recent committed & rolled back blocks from the state service #2639

Closed

jvff reviewed Aug 31, 2021

View reviewed changes

Clarify the difference between LatestChainTip and ChainTipChange

09f451e

teor2345 mentioned this pull request Sep 1, 2021

Reset ChainTipChanges on chain fork and network upgrade activation #2721

Merged

3 tasks

Merge branch 'main' into chain-change-grow

1cf3a08

jvff approved these changes Sep 1, 2021

View reviewed changes

Merge branch 'main' into chain-change-grow

2d7367e

teor2345 enabled auto-merge (squash) September 1, 2021 21:54

teor2345 merged commit b6fe816 into main Sep 1, 2021

teor2345 deleted the chain-change-grow branch September 1, 2021 22:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `ChainTipChange` type to `await` chain tip changes #2715

Add a `ChainTipChange` type to `await` chain tip changes #2715

teor2345 commented Aug 31, 2021

jvff left a comment •

edited

Loading

jvff Aug 31, 2021

teor2345 Aug 31, 2021

jvff commented Aug 31, 2021 •

edited

Loading

teor2345 commented Aug 31, 2021 •

edited

Loading

teor2345 commented Aug 31, 2021

teor2345 commented Sep 1, 2021

teor2345 commented Sep 1, 2021

jvff commented Sep 1, 2021

Add a ChainTipChange type to await chain tip changes #2715

Add a ChainTipChange type to await chain tip changes #2715

Conversation

teor2345 commented Aug 31, 2021

Motivation

Solution

Review

Reviewer Checklist

Follow Up Work

jvff left a comment • edited Loading

Choose a reason for hiding this comment

jvff Aug 31, 2021

Choose a reason for hiding this comment

teor2345 Aug 31, 2021

Choose a reason for hiding this comment

jvff commented Aug 31, 2021 • edited Loading

teor2345 commented Aug 31, 2021 • edited Loading

teor2345 commented Aug 31, 2021

teor2345 commented Sep 1, 2021

teor2345 commented Sep 1, 2021

jvff commented Sep 1, 2021

Add a `ChainTipChange` type to `await` chain tip changes #2715

Add a `ChainTipChange` type to `await` chain tip changes #2715

jvff left a comment •

edited

Loading

jvff commented Aug 31, 2021 •

edited

Loading

teor2345 commented Aug 31, 2021 •

edited

Loading